Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

manual: signature option for caml_example #1702

Merged
merged 6 commits into from May 5, 2018

Conversation

Octachron
Copy link
Member

This small PR extends the options of the caml_example pseudo-environment with a signature mode. With this PR, the possible modes for caml_example are:

  • toplevel, for examples that should look as if they were send to the toplevel
  • verbatim, for generic code examples
  • signature, for examples of interface files or signature

This seems to cover most documentation needs in the manual, and it is sufficient to convert the faulty example fixed by @lpw25 in #1693 .
In term of implementation, the code inside the environment is merely wraped inside module type Wrap = sig ... end before being send to the ocaml subprocess.

@gasche
Copy link
Member

gasche commented Apr 6, 2018

I am not sure whether the sig .. end wrapping mechanism works correctly (or whether it is going behave in weird/surprising ways in some cases; what if we use the mode that also shows the toplevel output?). I assume you have tested it by converting one of the signatures in the manual, and checked that the output is as you expect? If so, maybe this change could be part of the patch as well?

@Octachron
Copy link
Member Author

Octachron commented Apr 6, 2018

For more concrete example, I finished the conversion of the example in ocamldoc: html and pdf . (The corresponding commit also fixes transf.mll to make sure that \camlexample is considered a verbatim mode).

what if we use the mode that also shows the toplevel output?

In this case, it would show the wrapping module type, I can see two options to avoid this case: either require to combine the signature mode with caml_example * or remove the wrapping module type. I tend for the first option.

@gasche
Copy link
Member

gasche commented Apr 6, 2018

Yes, the first option is simplest, and inferred signatures are not very useful anyway (they mostly repeat the signature).

Would you consider adding a Changes entry, though? This is a nice change that did require some work from you, and I like the idea of crediting changes to the manual and broader documentation ecosystem.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved -- although I understand that you might want to make further tweaks. Feel free to merge whenever you are ready and the CI passes.

(This change caught other mistakes in the ocamldoc manual than the ones caught by Leo, which means that it was indeed a reasonable idea.)

&& mv $*.transf_error.tex $*.gen.tex\
&& $(TEXQUOTE) < $*.gen.tex > $*.texquote_error.tex\
&& mv $*.texquote_error.tex $*.tex\
|| printf "Failure when generating %s\n" $*.tex
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are accumulating logic to use CAMLLATEX/TRANSF/TEXQUOTE in several different Makefiles (apparently cmds/Makefile does not use CAMLLATEX but only the other two?). Maybe it would make sense to factorize this in a shared makefile, or as a separate shellscript to invoke?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably a good idea for a next PR.

@Octachron
Copy link
Member Author

I added an error message for the combination of \begin{caml_example}{signature}, and a change entry. I will merge after the merge of Leo's doctring fix.

@gasche
Copy link
Member

gasche commented May 5, 2018

I think we could keep this in 4.07, as it could be useful for further improve-the-manual changes (eg. a more complete version of #1209) that it could be worthwhile to include in 4.07 documentation. The present PR does not touch anything that is critical for the release preparation/testing, and it is customary to give external tools (ocamldoc, the manual build tools) more slack in freeze decisions.

@Octachron
Copy link
Member Author

It is true that this is mostly a validation tool. I moved back the change entry to 4.07 .

@@ -12,9 +12,14 @@ TRANSF=$(SET_LD_PATH) $(OCAMLRUN) ../../tools/transf
TEXQUOTE=../../tools/texquote2
FORMAT=../../tools/format-intf

WITH_TRANSF= ocamldoc.tex top.tex intf-c.tex flambda.tex spacetime.tex \
CAMLLATEX= $(OCAMLRUN) ../../tools/caml-tex2 -caml "TERM=norepeat $(OCAML)" \
-n 80 -v false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this not need an update to take 482e8a8 into account?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are totally right, I had forgotten this point. I should try to make my test environment sensitive to this problem. Fixed.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to merge when CI passes.

@Octachron
Copy link
Member Author

I removed a wandering unused macros, since keeeping the macro count as low as possible seemed important enough to warrant a last minute fix. I will squash and merge once the CI passes.

@Octachron Octachron merged commit 145bedc into ocaml:trunk May 5, 2018
Octachron added a commit that referenced this pull request May 5, 2018
* Signature option for caml_example
* convert ocamldoc code example to caml_example
* error message when using caml_example*{signature} without *
@Octachron
Copy link
Member Author

Cherry-picked to 4.07 as d040ad6

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants